mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)

591-605: ⚠️ Potential issue | 🟠 Major

Reintroduce an explicit insertion-point guard before building the merged chain.

All arith::/math:: ops for the merged quaternion are created at the current rewrite insertion point, which is still the chain head here. If a later gate angle is produced by an arith op between chain elements, the new ops will be inserted before that definition and violate SSA dominance. Please move the insertion point to chain.back().getOperation() (or just before the first post-chain consumer) while materializing constants, qAccum, and newOp.

🐛 Proposed fix

     auto chain = collectChain(op);
     if (chain.size() < 2) {
       return failure();
     }
 
+    OpBuilder::InsertionGuard guard(rewriter);
+    rewriter.setInsertionPoint(chain.back().getOperation());
+
     auto loc = op->getLoc();
     auto constants = createConstants(loc, rewriter);
 
     // Initialize quaternion accumulator from the first operation
     auto qAccum = quaternionFromRotation(chain.front(), constants, rewriter);

Run the following script to confirm the missing insertion-point adjustment and the lack of a regression test with late-defined scalar angles:

#!/bin/bash
set -euo pipefail

echo "=== matchAndRewrite body ==="
fd QuaternionMergeRotationGates.cpp --exec sed -n '580,610p' {}

echo
echo "=== quaternion-merge tests using arithmetic between mergeable gates ==="
fd test_qco_quaternion_merge.cpp --exec rg -n 'arith\.(addf|subf|mulf|divf)' {}

Expected result: matchAndRewrite shows no setInsertionPoint* call before building constants/qAccum/newOp, and the test search returns no coverage for a late-defined angle value.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 591 - 605, The merged-quaternion ops are currently created at the
rewrite's existing insertion point (head of the chain), risking SSA dominance if
a later gate angle is produced by an arith op; before calling createConstants,
quaternionFromRotation (for the first op), the loop that folds with
hamiltonProduct, and before uOpFromQuaternion, set the rewriter insertion point
to chain.back().getOperation() (or the operation immediately before the first
post-chain consumer) so all arith::/math:: ops for the merged quaternion are
emitted after the chain elements; locate these calls (createConstants,
quaternionFromRotation, hamiltonProduct, uOpFromQuaternion) and wrap them with a
temporary insertion-point guard (setInsertionPoint / setInsertionPointAfter or
equivalent) and restore the original insertion point after newOp is created.

mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)

555-568: 🧹 Nitpick | 🔵 Trivial

Remove the stale pairwise merge helper.

createOpQuaternionMergedAngle is no longer referenced after the chain-based rewrite. Keeping a second merge path here makes future fixes easy to miss, and Cppcheck is already flagging it as unused.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 555 - 568, Remove the now-unused pairwise merge helper by deleting
the createOpQuaternionMergedAngle function definition (and any corresponding
forward declaration) and any code that references it; locate the function
signature using the symbols UnitaryOpInterface and PatternRewriter and the body
that calls quaternionFromRotation, hamiltonProduct, and uOpFromQuaternion, then
remove that entire helper to avoid a duplicate merge path and silence the
Cppcheck unused warning.

mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp (1)

132-141: ⚠️ Potential issue | 🟡 Minor

Compare U angles modulo periodicity to prevent false-negative tests.

expectUGateParams currently does direct numeric comparison, but these parameters are periodic; the identity test even documents two equivalent forms (phi = 0 or 2π) while asserting only one. This can fail on equivalent outputs.

♻️ Proposed fix

+  static double circularDiff(double a, double b, double period) {
+    const double diff = std::fmod(std::abs(a - b), period);
+    return std::min(diff, period - diff);
+  }
+
   void expectUGateParams(double expectedTheta, double expectedPhi,
                          double expectedLambda, double tolerance = 1e-8) {
     auto params = getUGateParams();
     ASSERT_TRUE(params.has_value());
 
     auto [theta, phi, lambda] = *params;
-    EXPECT_NEAR(theta, expectedTheta, tolerance);
-    EXPECT_NEAR(phi, expectedPhi, tolerance);
-    EXPECT_NEAR(lambda, expectedLambda, tolerance);
+    EXPECT_NEAR(circularDiff(theta, expectedTheta, 4.0 * PI), 0.0, tolerance);
+    EXPECT_NEAR(circularDiff(phi, expectedPhi, TAU), 0.0, tolerance);
+    EXPECT_NEAR(circularDiff(lambda, expectedLambda, TAU), 0.0, tolerance);
   }

Based on learnings: In MQT Core’s OpenQASM 2 U definition, theta is 4π periodic while phi and lambda are 2π periodic.

Also applies to: 741-754

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp`
around lines 132 - 141, The test's expectUGateParams does direct numeric
comparisons causing false negatives for equivalent U gates; update
expectUGateParams (and the other similar assertions around the second block
referencing U parameters) to compare angles modulo periodicity: normalize theta
differences modulo 4π and phi/lambda modulo 2π (e.g., compute minimal
wrap-around difference between expected and actual angle and then use
EXPECT_NEAR on that minimal difference with the given tolerance); locate
getUGateParams usage and replace the three EXPECT_NEAR checks with
wrapped-comparison logic so equivalent angles like 0 vs 2π (or theta differing
by 4π) no longer fail.

mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (3)

601-608: ⚠️ Potential issue | 🟠 Major

Renormalize qAccum before converting back to Euler angles.

uOpFromQuaternion assumes a unit quaternion, but this path now chains several Hamilton products without the old quaternion→Euler→quaternion reset. Floating-point drift in qAccum will skew beta, and near the gimbal-lock thresholds it can also flip the safe branch. Normalize once before Line 608.

➕ Suggested fix

   for (auto chainOp : llvm::drop_begin(chain)) {
     auto qi = quaternionFromRotation(chainOp, constants, rewriter);
     qAccum = hamiltonProduct(qi, qAccum, loc, rewriter);
   }
+  qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter);

   // Convert merged quaternion back to UOp
   auto newOp = uOpFromQuaternion(qAccum, op, constants, rewriter);

static Quaternion normalizeQuaternion(Quaternion q, Location loc,
                                      const Constants& constants,
                                      PatternRewriter& rewriter) {
  auto ww = arith::MulFOp::create(rewriter, loc, q.w, q.w);
  auto xx = arith::MulFOp::create(rewriter, loc, q.x, q.x);
  auto yy = arith::MulFOp::create(rewriter, loc, q.y, q.y);
  auto zz = arith::MulFOp::create(rewriter, loc, q.z, q.z);

  auto sum1 = arith::AddFOp::create(rewriter, loc, ww, xx);
  auto sum2 = arith::AddFOp::create(rewriter, loc, yy, zz);
  auto normSq = arith::AddFOp::create(rewriter, loc, sum1, sum2);
  auto norm = math::SqrtOp::create(rewriter, loc, normSq);
  auto invNorm = arith::DivFOp::create(rewriter, loc, constants.one, norm);

  return {
      .w = arith::MulFOp::create(rewriter, loc, q.w, invNorm),
      .x = arith::MulFOp::create(rewriter, loc, q.x, invNorm),
      .y = arith::MulFOp::create(rewriter, loc, q.y, invNorm),
      .z = arith::MulFOp::create(rewriter, loc, q.z, invNorm),
  };
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 601 - 608, Normalize qAccum before converting back to Euler angles:
insert a call to a new helper (e.g., normalizeQuaternion) that takes the
Quaternion qAccum, Location loc, Constants& constants, and PatternRewriter&
rewriter, computes its magnitude via w^2+x^2+y^2+z^2, sqrt, and multiplies each
component by 1/norm to produce a unit quaternion, then pass the normalized
result into uOpFromQuaternion instead of raw qAccum; implement
normalizeQuaternion returning a Quaternion and call it just before the
uOpFromQuaternion(qAccum, op, constants, rewriter) invocation.

---

595-608: ⚠️ Potential issue | 🔴 Critical

Build the merged quaternion where every later parameter dominates.

Line 603 can consume angle SSA values from gates that appear after chain.front(), but the current insertion point is still at the chain head. That emits arith/math ops using non-dominating operands as soon as a later rotation angle is computed between the head and tail, which makes the rewritten IR invalid. Move the whole merge computation under an insertion guard to a point before chain.back().

🛠️ Suggested fix

   auto loc = op->getLoc();
-  auto constants = createConstants(loc, rewriter);
+  OpBuilder::InsertionGuard guard(rewriter);
+  rewriter.setInsertionPoint(chain.back().getOperation());
+  auto constants = createConstants(loc, rewriter);

   // Initialize quaternion accumulator from the first operation
   auto qAccum = quaternionFromRotation(chain.front(), constants, rewriter);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 595 - 608, The merge loop computes and emits arithmetic using SSA
values from later gates while the rewriter insertion point is still at the chain
head; move the entire quaternion-merge computation (creation of constants,
quaternionFromRotation calls, hamiltonProduct folding, and uOpFromQuaternion)
under an insertion-point guard so all new ops are inserted at a location that
dominates all consumed angle SSA values — specifically set the rewriter
insertion point to just before chain.back() (or use a
ScopedInsertionPoint/rewriter.setInsertionPoint before chain.back()) before
calling createConstants, quaternionFromRotation, hamiltonProduct, and
uOpFromQuaternion to ensure later parameters dominate.

---

547-572: 🧹 Nitpick | 🔵 Trivial

Remove the stale pairwise merge helper.

createOpQuaternionMergedAngle no longer has callers after the chain-based rewrite landed, so it is now dead code and a second merge path that can drift from matchAndRewrite. Please delete it instead of maintaining both implementations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 547 - 572, Delete the now-dead pairwise merge helper function
createOpQuaternionMergedAngle and any forward declarations or declarations
referring to it so only the chain-based merge remains; remove the entire static
function body (and any comments immediately attached) from
QuaternionMergeRotationGates.cpp, search for and eliminate any remaining
references to createOpQuaternionMergedAngle (including tests or headers) so
compilation still succeeds and matchAndRewrite remains the single merge
implementation.

mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)

574-581: 🧹 Nitpick | 🔵 Trivial

Consider renormalizing the accumulated quaternion before Euler extraction.

After chaining multiple Hamilton products (lines 575-578), floating-point drift can move |qAccum| slightly off 1.0. While the acos input clamping at lines 481-484 partially mitigates this for beta, explicit renormalization would provide additional robustness for longer chains:

// After the Hamilton product loop:
// qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter);

This is a minor robustness enhancement rather than a correctness bug—for typical chain lengths (2-5 gates), the accumulated error is negligible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 574 - 581, After accumulating quaternions with
quaternionFromRotation and hamiltonProduct into qAccum, normalize qAccum before
converting back to a UOp to avoid FP drift: insert a call to a quaternion
normalization helper (e.g., normalizeQuaternion that computes the quaternion
norm and divides components using constants/rewriter and loc) immediately after
the Hamilton product loop and before uOpFromQuaternion(qAccum, op, constants,
rewriter); if no normalizeQuaternion exists, add one that uses the existing
constants/rewriter pattern to compute 1.0 / sqrt(sumsq) and multiply qAccum
components by that scalar.

mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (2)

571-580: ⚠️ Potential issue | 🟠 Major

Renormalize qAccum before converting it back to Euler angles.

uOpFromQuaternion assumes a unit quaternion, but Lines 574-577 now fold an arbitrary-length Hamilton-product chain without reprojecting onto the unit sphere. That drift skews beta and can flip the gimbal-lock branch near 0/PI. A small normalizeQuaternion(...) helper that divides by sqrt(w*w + x*x + y*y + z*z) is enough here.

♻️ Proposed fix

     for (auto chainOp : llvm::drop_begin(chain)) {
       auto qi = quaternionFromRotation(chainOp, constants, rewriter);
       qAccum = hamiltonProduct(qi, qAccum, loc, rewriter);
     }
+    qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter);
 
     // Convert merged quaternion back to UOp
     auto newOp = uOpFromQuaternion(qAccum, op, constants, rewriter);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 571 - 580, After folding the chain into qAccum via
quaternionFromRotation and hamiltonProduct, renormalize qAccum before converting
back to Euler with uOpFromQuaternion: add or call a normalizeQuaternion(qAccum)
helper that divides each component by sqrt(w*w + x*x + y*y + z*z) (and
defensively handle zero/nan norm), then pass the normalized quaternion to
uOpFromQuaternion to avoid drift and gimbal-lock flips.

---

567-580: ⚠️ Potential issue | 🔴 Critical

Move the rewrite insertion point to the chain tail before building the quaternion IR.

Line 568 still creates the helper arith/math ops at the pattern’s current insertion point. If a later gate in the chain gets its angle from an SSA value defined between the head and tail, those new ops will reference that value before it dominates, and the rewrite emits invalid IR. Use an InsertionGuard and set the insertion point to chain.back().getOperation() before createConstants, quaternionFromRotation, and uOpFromQuaternion, then add a regression where the second angle is produced by an arith.addf between two mergeable gates.

♻️ Proposed fix

   auto loc = op->getLoc();
+  OpBuilder::InsertionGuard guard(rewriter);
+  rewriter.setInsertionPoint(chain.back().getOperation());
   auto constants = createConstants(loc, rewriter);

#!/bin/bash
echo "=== matchAndRewrite insertion point ==="
sed -n '556,586p' mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
echo
echo "=== current test coverage for SSA-computed angle values ==="
rg -n "arith\\.(addf|subf|mulf|divf)|builder\\.(rx|ry|rz|p|r|u2|u)\\(" mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp -C2

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 567 - 580, The rewrite currently creates helper arith/math ops at
the pattern's original insertion point, which can break SSA dominance; wrap the
rewrite with an mlir::OpBuilder::InsertionGuard and call
rewriter.setInsertionPointAfter(chain.back().getOperation()) (or
setInsertionPoint(chain.back().getOperation())) before calling createConstants,
quaternionFromRotation, and uOpFromQuaternion so all helper ops are emitted at
the chain tail; update the code locations around
qAccum/quaternionFromRotation/hamiltonProduct/uOpFromQuaternion to use the
guarded inserter and add a unit test where the second gate's angle is computed
via an arith.addf between two mergeable gates to verify correct dominance.

mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (2)

193-201: ⚠️ Potential issue | 🟡 Minor

Restore the trailing llvm_unreachable() in createAxisQuaternion.

The switch is exhaustive for today's enum, but this function still has a no-return path. That can reintroduce -Wreturn-type noise on some compilers and becomes UB if RotationAxis ever grows.

♻️ Suggested fix

     case RotationAxis::Z:
       return {.w = cos, .x = constants.zero, .y = constants.zero, .z = sin};
     }
+    llvm_unreachable("Unhandled RotationAxis");
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`
around lines 193 - 201, The switch in createAxisQuaternion returns for all
current RotationAxis values but leaves a theoretical no-return path; restore a
final llvm_unreachable() (or equivalent assert) after the switch in
createAxisQuaternion to silence -Wreturn-type and prevent UB if RotationAxis is
extended in the future, ensuring the function always has a terminal unreachable
statement for the default/unhandled case.

---

85-94: 🧹 Nitpick | 🔵 Trivial

Use MLIR RTTI instead of getName() for the mergeability check.

This predicate is defining chain boundaries, so coupling it to textual op names makes it more brittle than the isa/TypeSwitch style you already use everywhere else in this file.

mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp (1)

125-137: ⚠️ Potential issue | 🟡 Minor

Compare U parameters modulo their natural periods in the helper.

expectUGateParams currently treats equivalent outputs like 0 vs 2π or π vs -π as different, so a mathematically correct rewrite can fail these tests after harmless canonicalization changes.

♻️ Suggested fix

+  static double periodicDiff(double actual, double expected, double period) {
+    return std::abs(std::remainder(actual - expected, period));
+  }
+
   void expectUGateParams(double expectedTheta, double expectedPhi,
                          double expectedLambda, double tolerance = 1e-8) {
     auto params = getUGateParams();
     ASSERT_TRUE(params.has_value());
 
     auto [theta, phi, lambda] = *params;
-    EXPECT_NEAR(theta, expectedTheta, tolerance);
-    EXPECT_NEAR(phi, expectedPhi, tolerance);
-    EXPECT_NEAR(lambda, expectedLambda, tolerance);
+    EXPECT_NEAR(periodicDiff(theta, expectedTheta, 4.0 * PI), 0.0, tolerance);
+    EXPECT_NEAR(periodicDiff(phi, expectedPhi, 2.0 * PI), 0.0, tolerance);
+    EXPECT_NEAR(periodicDiff(lambda, expectedLambda, 2.0 * PI), 0.0,
+                tolerance);
   }

Based on learnings, in MQT Core the U gate uses the OpenQASM 2 definition, so θ is 4π-periodic while φ and λ are 2π-periodic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp`
around lines 125 - 137, The helper expectUGateParams should compare angles
modulo their natural periods instead of raw values: retrieve params via
getUGateParams(), unwrap the optional as done now, then normalize theta to the
canonical range modulo 4π and normalize phi and lambda modulo 2π (e.g., map to
[-period/2, period/2] or [0,period) consistently) before calling EXPECT_NEAR;
update expectUGateParams to perform these modular reductions so equivalent
angles like 0 vs 2π or π vs -π are treated as equal.